Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce right types for spec-reserved keys #39

Merged
merged 21 commits into from
Aug 2, 2023

Conversation

armaganyildirak
Copy link
Member

Issue Addressed

Addresses #26

Proposed Changes

Checking key and value according to spec-reserved keys.

Additional Info

I have not finished the PR yet. I am still not sure about that I am following the correct idea.

@divagant-martian divagant-martian self-requested a review July 5, 2023 20:36
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@divagant-martian
Copy link
Collaborator

Hi @armaganyildirak just checked -after my review- that this PR is not finished. Two things that you can do to signal this in a more clear way (one, the other or both):

  • Add "WIP" (as in work in progress) in the title as follows: [WIP] Enforce right types for spec-reserved keys. Remove the tag when you are happy with the code.
  • Open the PR as a draft: I'll convert as a such shortly.

If you want a partial review, while still in progress, keep it as a draft/tagged as WIP and re-request or tag me/someone who you think could help, like age.
Mark as ready once you are satisfied with it and re-request reviews.

Regarding the code itself, to begin with, you can simply follow the same path taken for the u16 keys. The logic is:

  • Check if the key belongs to the reserved ones via comparison (as you are currently doing) and then attempt to decode as the expected type. For the u16 keys right now it looks like:
        if is_keyof_u16(key.as_ref()) {
            rlp::decode::<u16>(&value).map_err(|err| EnrError::InvalidRlpData(err.to_string()))?;
        }

so you would, for and ipv6 key for example, do something like:

        if <here goes the check about the key being ipv6> {
            rlp::decode::<IPv6Addr>(&value).map_err(|err| EnrError::InvalidRlpData(err.to_string()))?;
        }

After this the code can probable be improved further, but this is a good first step

@divagant-martian divagant-martian marked this pull request as draft July 8, 2023 01:26
@divagant-martian divagant-martian changed the title Enforce right types for spec-reserved keys [WIP] Enforce right types for spec-reserved keys Jul 8, 2023
@armaganyildirak armaganyildirak marked this pull request as ready for review July 13, 2023 00:45
src/builder.rs Outdated Show resolved Hide resolved
src/builder.rs Outdated Show resolved Hide resolved
src/builder.rs Outdated Show resolved Hide resolved
@divagant-martian
Copy link
Collaborator

divagant-martian commented Jul 25, 2023

It's going to take some iterations before we get this right @armaganyildirak but don't get discouraged.

Right now, you have changed most (if not all) the functions of the builder to return a Result. This is not very ergonomic. It would be better to accept the values in the builder in those functions and reject them if wrong in the build one.

We would get things like this:

assert!(EnrBuilder::new("v4").add_value("ip6", "most definitely not an ipv6! :O").build(&key).is_err());

Now onto the build function itself: the way it works right now is not ideal. As you've noticed, you had to add the verification in two places, in the enr and the builder as well. We should have it only in the enr and rely on that exclusively, included the builder function. For this part, I probably have to think a bit more how to do it correctly, hence the "multiple interations" ahead.

In the meantime, can you restore the functions that were not Result to how they were and move the validation to the build one?

Also, it would be better to add a match statement instead of multiple == comparisons for the reserved keys.

About clippy, don't bother dealing with errors that do not relate to your code, I added the fixes for those in #42

Thanks for hanging in there

@divagant-martian divagant-martian changed the title [WIP] Enforce right types for spec-reserved keys Enforce right types for spec-reserved keys Aug 2, 2023
Copy link
Collaborator

@divagant-martian divagant-martian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is an overall improvement. I think we merge this and iterate later on.

Ty for the great work @armaganyildirak

@divagant-martian divagant-martian merged commit 2f9ed5c into sigp:master Aug 2, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants